Skip to content

fix(rest): lower public-picker filter rules to the grammar the ingress parses - #16673

Merged
os-sales merged 5 commits into
mainfrom
claude/issue-16581-public-picker-filter-lowering
Sep 7, 2026
Merged

fix(rest): lower public-picker filter rules to the grammar the ingress parses#16673
os-sales merged 5 commits into
mainfrom
claude/issue-16581-public-picker-filter-lowering

Conversation

@os-sales

@os-sales os-sales commented Sep 7, 2026

Copy link
Copy Markdown
Collaborator

Fixes #16581

GET /forms/:slug/lookup/:field answered 400 INVALID_FILTER for every non-empty search. The route composed its filter list out of ViewFilterRule objects — the { field, operator, value } dialect FormFieldPublicPickerSchema.filter declares in so many words ("Same { field, operator, value } dialect as list-view filters") — and put them on the findData filter slot, which accepts a FilterCondition object or a FilterArray and refuses everything else. The route's own q predicate is built in the same object shape, so the refusal never depended on an author declaring publicPicker.filter: only the degenerate empty-filter call could succeed. On an anonymous application form the picker is the only sensible way to choose a job, and there is no application-side workaround.

The direction

The ROUTE lowers ViewFilterRule objects to the parser's grammar. The declaring side already promises the object dialect on the authoring surface, so what changes is the side that failed to honour the promise.

  • findData was NOT taught a second dialect. That would maintain two filter grammars in the data layer permanently and spread the object shape to every findData caller.
  • The operator fold reuses normalizeFilterOperator from @objectstack/spec/ui (packages/spec/src/ui/view.zod.ts) — the fold ViewFilterRuleSchema.operator itself runs as its z.preprocess, exported precisely so producers "normalize stored metadata against the SAME canonical map the schema uses, instead of inventing a second dialect". No second alias table was written. This route reads STORED bodies, never re-parsed ones, so a row carrying a legacy spelling (notEquals, isNotEmpty, gt) folds exactly as the schema folds it — driven by a test.
  • Nothing under packages/spec/src/ is edited. The schema is imported, not changed.

Both branches are lowered

picker.filter (the declared rows) and q (the row the route builds itself) are composed and lowered together, ANDed explicitly. No rules still means no filter ([]), never an empty logical node the ingress would refuse — that degenerate call is pinned so the one shape that always worked keeps working.

A row the lowering cannot read as a rule is forwarded verbatim, so the ingress refuses the request exactly as before. That direction is deliberate and fail-CLOSED: a picker's static filter is often the only thing bounding what an anonymous visitor can search, and silently skipping a row nobody understood would answer 200 over an unfiltered table.

⭐ The discriminating control

"The route lowers correctly" and "the parser was loosened" produce identical greens and have opposite consequences, so an assertion that the object shape fed directly to the parser still answers 400 is kept, in two independent places:

  • public-form-lookup-filter-lowering.test.ts §3 drives the card's own control pair through GET /data/:object, same server, same object, same anonymity, only the filter's shape differing — object shape → 400 INVALID_FILTER; triple shape → 200 with the right rows. That pair is what attributes the failure to SHAPE rather than to permissions, anonymity or another part of the route.
  • rest-server-canonical-query-ast.test.ts §3's CONTROL keeps its frozen picker literal and asserts the ingress still refuses it. The literal is a historical record, so it is deliberately left as it was rather than "updated" to the lowered shape.

Both ablation legs below leave §3 green, which is what makes it a control rather than a restatement.

A second defect, fixed in place

Once the 400 was gone the route answered 200 {"data":[]} — an empty picker for every search. The handler read result.data ?? result.items and never result.records, which is the key findData returns ({ object, records, total, hasMore }) and the order this file's three other read sites already use. Bounded in-place repair: same handler, same defect class (the route speaking a shape the protocol layer does not), and the acceptance for this card is "200 with the RIGHT ROWS", which is unreachable without it. The legacy data / items / rows / bare-array aliases stay, and a test drives one.

It was invisible twice over: unreachable while every non-empty search 400'd, and unreachable in public-form-lookup-picker.test.ts, whose findData double answers { data: rows } — a shape the real protocol does not produce.

Verification

Implementation committed FIRST, then mutated; every mutation proven on disk by marker counts and blob hash, restored under an EXIT/INT/TERM trap with an absolute path, and the restore proven by a whole-tree git status --porcelain plus blob-hash equality against HEAD. ablation-dist-preflight run on both legs of both ablations.

leg mutation measured
A the lowering removed (filters = rules) 5 red — every row/status assertion of §1, §2, §4; the refusal observed is INVALID_FILTER. §3 stays GREEN
A restore rebuilt preflight --absent: marker absent from all 6 built files; tree clean
B the records read removed 6 red with a DIFFERENT signature — expected [] to deeply equal [...], i.e. 200 with an empty list, zero INVALID_FILTER. §3 stays GREEN
B restore rebuilt preflight --absent: marker absent from all 6 built files; tree clean

All readings below are taken on eecc4b0 — this branch with current origin/main merged in, so nothing here is measured on a tree nobody is on (the derivation warned STALE TREE before the merge and the warning is gone after it).

  • pnpm --filter @objectstack/rest test190 files / 3228 tests passed.
  • pnpm --filter @objectstack/rest typecheck — green, including check:test-typecheck (0 files / 0 errors / 0 pinned debt).
  • Gate union, node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack --commands, re-derived on the merged head and reconciled with --ran: 65 derived, 65 run, 0 NOT-MEASURED, 0 UNRUN. Two of them (check:dual-build-cjs-loads, check:type-check-debt) first reported exit 3 — PREREQUISITE NOT MET on a partially built tree; a full turbo run build across the workspace was run so both became real readings rather than a non-measurement.
  • Two gates named this PR's new suite and were satisfied rather than silenced: check:engine-double-contract (three additive rows regenerated with --write, no losses) and check:objectql-double-limit (the data double now holds the caller's bound, after the filter and by presence).

Changeset: patch on @objectstack/rest — a user-visible runtime repair on a published route, with no package graduating or breaking.

Clause ② — re-derived from the delivered diff: no

An external caller does observe 400 → 200 on a published route, but no declared face moves. FormFieldPublicPickerSchema.filter already declares the object dialect as accepted and its describe text already promises it; this makes the runtime honour what the declaration says. The diff touches packages/rest and one generated ledger under scripts/, and nothing under packages/spec/src/. Repairing a route that refuses what its own contract promises is a defect fix, not a contract change.

验收备注

Observed while working here, deliberately NOT changed and NOT filed:

  • packages/rest's picker suites build their findData doubles from response shapes the real protocol does not produce ({ data }, { items }). That is what hid the records read for as long as it hid it. A repo-wide "protocol double answers the protocol's own envelope" sweep is a class-closing job, not a rider on this card.
  • The lowering helper lives in packages/rest/src/view-filter-rule-lowering.ts because the ruling put the translation at the door that speaks both dialects. If a second consumer ever needs it, promoting it is its own card — it is not needed by anything today.
  • rest-server.ts emits an unused-import warning for zodIssuesToFields during tsup's ESM pass on main as well as here; pre-existing, unrelated to this diff, and not a build failure.

Generated by Claude Code

…s parses

`GET /forms/:slug/lookup/:field` composed its filter list out of
`ViewFilterRule` objects — the `{ field, operator, value }` dialect
`FormFieldPublicPickerSchema.filter` declares — and put them on the
`findData` filter slot, which accepts a `FilterCondition` or a
`FilterArray` and refuses everything else with `400 INVALID_FILTER`. The
`q` branch builds the same object shape itself, so every non-empty
search was refused whether or not an author declared
`publicPicker.filter`; only the degenerate empty-filter call succeeded.

The route now lowers the composed rows to the array grammar, reusing
`normalizeFilterOperator` from `@objectstack/spec/ui` rather than
restating the alias table. A row that cannot be read as a rule is
forwarded verbatim so the ingress still refuses the request — the
fail-closed direction on an anonymous surface, where the picker's static
filter is what bounds what a visitor can search.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YFY46JydE1gMxQG1TqBcMZ
The public lookup handler read `result.data ?? result.items` and never
`result.records`, which is the key `findData` returns
(`{ object, records, total, hasMore }`) and the order this file's three
other read sites already use. With the filter lowered the route
therefore answered `200 {"data":[]}` — an empty picker for every search,
the same user-visible outcome as the 400 by a different route.

Bounded in-place: same handler, same defect class (the route speaking a
shape the protocol layer does not), and the acceptance for #16581 is
"200 with the RIGHT ROWS", which is unreachable without it. The legacy
`data` / `items` / `rows` / bare-array aliases stay so protocol doubles
and alternate protocols keep working.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YFY46JydE1gMxQG1TqBcMZ
`check:engine-double-contract` and `check:objectql-double-limit` both
name the new suite: the metadata-write double's delete/findOne/update
seams were not in the pinned ledger (regenerated with `--write`, three
additive rows, no losses), and the data double returned its filtered
rows without applying `options.limit`.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YFY46JydE1gMxQG1TqBcMZ
@github-actions github-actions Bot added size/l documentation Improvements or additions to documentation tests tooling labels Sep 7, 2026
@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/rest, touching 3 documentable anchor(s). ⚠️ 1 changed file(s) yielded no anchor (packages/rest/src/rest-server.ts), so the pages documenting them are NOT COVERED by this run — this is not a clean bill of health for those files.

1 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:

  • content/docs/ui/forms.mdx (via /forms/:slug/lookup/:field (route, a path literal on a changed line))
What this run could not see
  • 1 changed file(s) yielded no anchor (packages/rest/src/rest-server.ts) — pages documenting those are invisible to this run
  • the SDK route bridge reached 61 of 219 client-bound route-ledger rows — the other 158 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run. Of those 158: 0 are remediable by widening that discovery convention (an in-repo file declares the path; the convention did not scan it); 56 are structural — on a ledger where NOT ONE row is declared in-repo, so no discovery change reaches them at any price; 102 are undecided (no in-repo declaration, on a ledger that has other in-repo registrars — absence and an unreadable spelling are not distinguishable here). The rows themselves: node scripts/docs-audit/affected-docs.mjs --bridge-coverage
  • a page that states a rule by its inputs shares no identifier with the emitter that implements the rule, so an emitter-only diff cannot list it — not on this run and not on any run. Measured on fix(driver-sql): emit varchar(maxLength) for a text field a declared index keys on #11430: content/docs/protocol/objectql/types.mdx documents the text-family column mapping by the ObjectQL type names it maps FROM (text / textarea / html) while the diff changed createColumn; it went unlisted, and it was the page that diff falsified, in four places. No shared token exists to detect this on, so a rule your change carries has to be re-read by hand in the pages that restate it.

Coarse fallback — 13 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json 2539f4aec60e327f5a9ff83e6d07f16054d876eepackageMentionDocs.

Which tree this was computed on

This run read content/docs from 21cc35fea1d633684324efe12ab6fd506e4399c6 — the merge of head eecc4b0722a14f8a515014516b3d987df336848b into base 2539f4aec60e327f5a9ff83e6d07f16054d876ee, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 21cc35fea1d633684324efe12ab6fd506e4399c6 && git checkout 21cc35fea1d633684324efe12ab6fd506e4399c6
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin 2539f4aec60e327f5a9ff83e6d07f16054d876ee eecc4b0722a14f8a515014516b3d987df336848b && git checkout -B drift-repro 2539f4aec60e327f5a9ff83e6d07f16054d876ee && git merge --no-ff eecc4b0722a14f8a515014516b3d987df336848b

node scripts/docs-audit/affected-docs.mjs --json 2539f4aec60e327f5a9ff83e6d07f16054d876ee

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs 2539f4aec60e327f5a9ff83e6d07f16054d876ee → pass the list as
args.docs, on the commit named under Which tree this was computed on.

@os-sales
os-sales marked this pull request as ready for review September 7, 2026 18:27
@os-sales
os-sales enabled auto-merge September 7, 2026 18:27
@os-sales
os-sales added this pull request to the merge queue Sep 7, 2026
Merged via the queue into main with commit d5c4022 Sep 7, 2026
35 checks passed
@os-sales
os-sales deleted the claude/issue-16581-public-picker-filter-lowering branch September 7, 2026 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/l tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Public lookup route builds object-shaped filter rules the data layer refuses: GET /forms/:slug/lookup/:field answers 400 INVALID_FILTER for every search

2 participants